-
Notifications
You must be signed in to change notification settings - Fork 65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
db_bench: fix for issue #290 #370
Conversation
Hi @andy-byers, |
Hey @udi-speedb, thanks for the feedback. I'll run |
I figured I would explain some of my reasoning behind these changes. First, in On a side note, it doesn't look like there are any tests in |
tools/db_bench_tool.cc
Outdated
@@ -2953,6 +2952,9 @@ class Benchmark { | |||
} else { | |||
auto wal_dir = options.wal_dir; | |||
for (int i = 0; i < FLAGS_num_multi_db; i++) { | |||
if (dbs_[i].db || dbs_[i].opt_txn_db) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please wrap the reference to opt_txn_db with #ifndef ROCKSDB_LITE / #endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally missed that, thank you. As for your comment below, you're right, the db
member needs to be nulled out unconditionally. Then the check for opt_txn_db
can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally missed that, thank you. As for your comment below, you're right, the
db
member needs to be nulled out unconditionally. Then the check foropt_txn_db
can be removed.
However, please do not change that as part of this PR. This is a RocksDB issue (=> upstreamable from our standpoint) and, therefore, should be handled in a separate issue (dbs_to_use_ is a Speedb only addition).
I will open a separate issue for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I just pushed a fix for this change. Does that look okay? It should be sufficient until the other issue is fixed, at which point the code can be simplified as discussed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I just pushed a fix for this change. Does that look okay? It should be sufficient until the other issue is fixed, at which point the code can be simplified as discussed.
Thanks. I only now thought about it, but couldn't you just replace the following block of code:
#ifndef ROCKSDB_LITE
if (FLAGS_optimistic_transaction_db) {
if (dbs_[i].opt_txn_db) {
continue;
}
} else if (dbs_[i].db) {
continue;
}
#else // ROCKSDB_LITE
if (dbs_[i].db) {
continue;
}
#endif // ROCKSDB_LITE
With this:
if (dbs_[i].db) {
continue;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I understand.
I have opened #374 to fix the RocksDB issue (nullifying db_
)
Please let me know if you would you like to add tests as part of this PR. If not, I will approve it as is. I can open another issue to add the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering, do you know how the benchmark tests are built and run? db_bench_tool_test
is provided as a target in the root Makefile, but isn't mentioned anywhere else that I could find. I built it and ran it (from main) with no arguments, and it actually trips some assertions. Is it supposed to be run with some arguments?
I think adding those tests might be a bit more complicated than I originally thought, and would probably be better off in a different issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not familiar with db_bench_test_tool so I can't help you there. I will look at it in the near future to get to know it.
Thanks so much for working on this issue.
Please squash your commits and also put the issue number in parenthesis in the commit message (#290). I will then approve it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to force push, but I got everything squashed and fixed the commit message.
Thank you for your help! I'll continue to look into the benchmark tests and file an issue if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much again for working on this.
Approving the PR.
@andy-byers Thanks for fixing the formatting issue and for the detailed explanation of your changes.
Shouldn't the |
@andy-byers As for your proposal to add tests in |
Changed db_bench so that when using multiple databases (num_multi_db > 1) we only destroy databases that were used for the current group (databases specified in dbs_to_use). Added logic in `OpenAllDbs()` to handle the case where the `-optimistic_transaction_db` option is used.
Changed db_bench so that when using multiple databases (num_multi_db > 1) we only destroy databases that were used for the current group (databases specified in dbs_to_use). Added logic in `OpenAllDbs()` to handle the case where the `-optimistic_transaction_db` option is used. Co-authored-by: andy-byers <andrew.byers@utdallas.edu>
Changed db_bench so that when using multiple databases (num_multi_db > 1) we only destroy databases that were used for the current group (databases specified in dbs_to_use). Added logic in `OpenAllDbs()` to handle the case where the `-optimistic_transaction_db` option is used. Co-authored-by: andy-byers <andrew.byers@utdallas.edu>
Changed db_bench so that when using multiple databases (num_multi_db > 1) we only destroy databases that were used for the current group (databases specified in dbs_to_use). Added logic in `OpenAllDbs()` to handle the case where the `-optimistic_transaction_db` option is used. Co-authored-by: andy-byers <andrew.byers@utdallas.edu>
Changed db_bench so that when using multiple databases (num_multi_db > 1) we only destroy databases that were used for the current group (databases specified in dbs_to_use). Added logic in `OpenAllDbs()` to handle the case where the `-optimistic_transaction_db` option is used. Co-authored-by: andy-byers <andrew.byers@utdallas.edu>
Changed db_bench so that when using multiple databases (num_multi_db > 1) we only destroy databases that were used for the current group (databases specified in dbs_to_use). Added logic in `OpenAllDbs()` to handle the case where the `-optimistic_transaction_db` option is used. Co-authored-by: andy-byers <andrew.byers@utdallas.edu>
Changed db_bench so that when using multiple databases (num_multi_db > 1) we only destroy databases that were used for the current group (databases specified in dbs_to_use).